Lastuse proofread #1
Conversation
|
I didnt do the |
| List(new InstrumentCoverage) :: // Perform instrumentation for code coverage (if -coverage-out is set) | ||
| List(new CrossVersionChecks, // Check issues related to deprecated and experimental | ||
| new FirstTransform, // Some transformations to put trees into a canonical form | ||
| new VerifyLastUseAnnotations, // Well actually it has its own tree traverser, so it does not make that much sense to be here |
There was a problem hiding this comment.
nit: You should usually add new phases at the end of the mega phase list
|
|
||
| object VerifyLastUseAnnotations: | ||
| val name: String = "verifyLastUseAnnotations" | ||
| val description: String = "verify that @lastUse annotations are use correctly" |
There was a problem hiding this comment.
| val description: String = "verify that @lastUse annotations are use correctly" | |
| val description: String = "verify that @lastUse annotations are used correctly" |
| val name: String = "verifyLastUseAnnotations" | ||
| val description: String = "verify that @lastUse annotations are use correctly" | ||
|
|
||
| case class VariableStatus(allowVar: Boolean, allowAnnot: Boolean, isAfterValDef: Boolean /*find better name*/){ |
There was a problem hiding this comment.
question: Could these fields be expressed in terms of the "state/status" of the variable, instead of actions that are allowed? e.g. (wasDeclared, wasNullified, etc.)
IMHO the cleanest way is for fields to mean "state" and for methods to mean "capability to do an action".
| //TODO find a better name | ||
| type VariableStatusMap = Map[Symbol, VariableStatus] | ||
|
|
||
| extension(ss: VariableStatusMap) |
There was a problem hiding this comment.
nit(personal preference): conventionally, we usually make a space after a keyword (here extension)
Coincidentally, GH syntax highlighting breaks because of that here.
| /* | ||
| Verifies the use of the @lastUse annotation | ||
| following rules are applied: | ||
| 1. the variable cannot be reused after being annotated | ||
| 2. the variable cannot be annotated in a loop (or in a lambda) TODO modify | ||
| 3. exclusive branches (if-else, match cases) do not impact each other. | ||
| the implementation goes as follows (considering nested functions): | ||
| we follow all variables with a @lastUse annotation | ||
| PrepareForDefDef: the miniphase dives in the deepest method in the tree, storing in context the lastUse variable from enclosing methods ("captured") | ||
| transformDefDef: each method (children first) are verified. If it contains a lastUse captured variable, it is marked as such (in "capturedVariables") | ||
| this allows illegal function calls to be detected, but all function definitions stay legal. | ||
| */ |
There was a problem hiding this comment.
style: You should use the doc comment syntax – /** */. link: https://docs.scala-lang.org/style/scaladoc.html
|
|
||
|
|
||
| override def prepareForDefDef(tree: DefDef)(using Context): Context = | ||
| val freeVars = ctx.property(lastUseVariables).getOrElse(Set.empty[Symbol]) ++ tree.getAttachment(methodLastUses).getOrElse(Set.empty[Symbol]) |
There was a problem hiding this comment.
question: are those actually free variables? Isn't it just the set of all lastUse variables?
| the implementation goes as follows (considering nested functions): | ||
| we follow all variables with a @lastUse annotation | ||
| PrepareForDefDef: the miniphase dives in the deepest method in the tree, storing in context the lastUse variable from enclosing methods ("captured") | ||
| transformDefDef: each method (children first) are verified. If it contains a lastUse captured variable, it is marked as such (in "capturedVariables") | ||
| this allows illegal function calls to be detected, but all function definitions stay legal. |
There was a problem hiding this comment.
nit: You probably shouldn't include implementation details in doc comments.
| private val lastUseVariables = Property.Key[Set[Symbol]] | ||
| private val capturedVariables = mutable.Map.empty[Symbol, Set[Symbol]] //method => lastUse symbols from outside it is using |
There was a problem hiding this comment.
style: Can we change the naming, so that the meaning of all the keys and maps is more clear?
Also is my intuition/understanding correct that? That:
lastUseVariables=all lastUse variables in scopecapturedVariables=for a given method -> captured lastUse variablesmethodLastUses=for a given method -> variables lastUsed in its body- plus concaptually, we also use the set of lastUse variables defined in this method
|
|
||
| if (tree.tpe.typeSymbol.isPrimitiveValueClass || tree.symbol.is(Flags.Method)) | ||
| report.error("`@lastUse` annotation cannot be used on primitives and methods", tree.sourcePos) | ||
| if tree.symbol.owner != method.symbol && !tree.isInstanceOf[This] then |
There was a problem hiding this comment.
question: Will this break for nested variables? Don't we conceptually also want enclosingMethod here?
There was a problem hiding this comment.
style: You really seem to like line breaks ^^
No description provided.